Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Full Sync :: limit to contributors and above #21648

Merged
merged 6 commits into from
Nov 5, 2021
Merged

Full Sync :: limit to contributors and above #21648

merged 6 commits into from
Nov 5, 2021
+41 −27

Conversation

mdbitz
Copy link
Contributor

@mdbitz mdbitz commented Nov 4, 2021

Reduce included users in Full Syncs to contributor and above users. This is performed by an explicit check of the {$wpdb->prefix}user_level meta which correlates to a non 0 value.

Fixes https://wp.me/p9dueE-3JV

Changes proposed in this Pull Request:

  • Limit the scope of User Full Syncs to contributor and above users.

Does this pull request change what data or activity we track or use?

NO

Testing instructions:

Create several users on your site, subscribers, contributors, admins, etc
Pre applying patch

  • wp shell
  • $um = Automattic\Jetpack\Sync\Modules::get_module( 'full-sync' ); // get full sync module.
  • $um->start( [ 'users'=>1 ] ); // initiate a full sync of users.
  • $um->get_status(); // returns current status
  • // Verify that the "total" for users matches # of users on the site regardless of capabilities.

Post applying patch

  • wp shell
  • $um = Automattic\Jetpack\Sync\Modules::get_module( 'full-sync' ); // get full sync module.
  • $um->start( [ 'users'=>1 ] ); // initiate a full sync of users.
  • $um->get_status(); // returns current status
  • // Verify that the "total" for users matches # of users on the site with contributor or above role

@mdbitz mdbitz added this to the jetpack/10.4 milestone Nov 4, 2021
@mdbitz mdbitz self-assigned this Nov 4, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2021 •
edited
Loading

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ All commits were linted before commit.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

Jetpack plugin:

  • Next scheduled release: December 7, 2021.
  • Scheduled code freeze: November 30, 2021.

@mdbitz
Copy link
Contributor Author

mdbitz commented Nov 4, 2021

Full Sync tests need overhaul:

  1. WP_Test_Jetpack_Sync_Full_Immediately::test_full_sync_sends_all_users
    Failed asserting that 1 matches expected 11.

  2. WP_Test_Jetpack_Sync_Full_Immediately::test_full_sync_doesnt_send_deleted_users
    Failed asserting that 1 matches expected 2.

  3. WP_Test_Jetpack_Sync_Full_Immediately::test_initial_sync_doesnt_sync_subscribers
    Failed asserting that 3 matches expected 13.

@github-actions github-actions bot added the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label Nov 4, 2021
@mdbitz mdbitz added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Nov 4, 2021
kraftbj
kraftbj reviewed Nov 4, 2021
View reviewed changes
projects/plugins/jetpack/tests/php/sync/test_class.jetpack-sync-full-immediately.php
@@ -1026,9 +1031,9 @@ function test_full_sync_can_sync_individual_comments() {
}

function test_full_sync_can_sync_individual_users() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a test here to confirm that a less-than-contributor is not included?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test_full_sync_sends_all_users creates a subscriber and verifies via the count that it doesn't get included.

I'm of the opinion that new tests have diminishing returns due to Full Sync being replaced with checksums.

@mdbitz mdbitz added [Status] In Progress and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 4, 2021
@mdbitz mdbitz added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Nov 4, 2021
@mdbitz mdbitz enabled auto-merge (squash) November 5, 2021 14:50
leogermani
leogermani approved these changes Nov 5, 2021
View reviewed changes
Copy link
Contributor

@leogermani leogermani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with the provided instructions and it worked as expected.

It's worth noting that user_level is "semi" deprecated. Something that's (almost) not used in core anymore... but it's still there and kept up to date in the user meta table.

It's updated based on the higher level of the level_%d capabilities ( where %d goes from 0 to 10). All core roles still declare these caps and I checked that Woocommerce also does.

Other custom roles might not do it and will therefore be kept out of sync.

@mdbitz mdbitz merged commit 3830aa4 into master Nov 5, 2021
@mdbitz mdbitz deleted the fix/full-sync-user_level branch November 5, 2021 22:10
@github-actions github-actions bot removed the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Nov 5, 2021
@mdbitz
Copy link
Contributor Author

mdbitz commented Nov 5, 2021

Yes user_level is "semi" deprecated however it still provides the most reasonable level for this limitation. We can't use "edit_post" or other single capabilities as it doesn't align fully with customizations. A non 0 level still is best way to exclude subscribers and other custom roles that don't have an "edit" / contributor role.

davidlonjon added a commit that referenced this pull request Nov 9, 2021
@davidlonjon
Merge branch 'master' into fix/boost-and-general-linting-formatting
c9739d5 
# By renovate[bot] (3) and others
# Via GitHub
* master:
  Update dependency preact to v10.5.15 (#21635)
  Search: Migrate helper classes to standalone package (#21552)
  Release script: fix small typo (#21675)
  Update babel monorepo (#21668)
  Lock file maintenance (#21664)
  E2E tests: clean unused dependencies (#21639)
  Create JS Config package (#21592)
  Use webpack config JS package in Jetpack (#21581)
  add test runner to dev dependencies (#21591)
  Pexels Integration: Make the initial image search for example query run only once (#21641)
  Boost: make progress bar corners round on small screens (#21626)
  Full Sync :: limit to contributors and above (#21648)
  Sync Checksums : limit users based on user_level (#21645)
  Storybook: add Jetpack Spinner (#21511)
  Contact Info and Map widget: widget deprecation (#21554)
  Use webpack config js package in everything except Jetpack (#21580)

# Conflicts:
#	pnpm-lock.yaml
#	projects/plugins/boost/package.json
davidlonjon added a commit that referenced this pull request Nov 9, 2021
@davidlonjon
Merge branch 'master' into add/boost-e2e-ci-integration
e6ae4ff 
* master: (24 commits)
  Update dependency preact to v10.5.15 (#21635)
  Search: Migrate helper classes to standalone package (#21552)
  Release script: fix small typo (#21675)
  Update babel monorepo (#21668)
  Lock file maintenance (#21664)
  E2E tests: clean unused dependencies (#21639)
  Create JS Config package (#21592)
  Use webpack config JS package in Jetpack (#21581)
  add test runner to dev dependencies (#21591)
  Pexels Integration: Make the initial image search for example query run only once (#21641)
  Boost: make progress bar corners round on small screens (#21626)
  Full Sync :: limit to contributors and above (#21648)
  Sync Checksums : limit users based on user_level (#21645)
  Storybook: add Jetpack Spinner (#21511)
  Contact Info and Map widget: widget deprecation (#21554)
  Use webpack config js package in everything except Jetpack (#21580)
  Boost: Fix the official name to the JavaScript word reference (#21638)
  tools: Allow extra spaces in next-version tags (#21647)
  Publicize: always add an alt text to Open Graph Images (#21520)
  js-tools: Add missing eslint plugin dep (#21642)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kraftbj kraftbj kraftbj left review comments

@leogermani leogermani leogermani approved these changes

Assignees

@mdbitz mdbitz

Labels
[Package] Sync [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/
Projects
None yet
Milestone
jetpack/10.4
Development

Successfully merging this pull request may close these issues.
3 participants
@mdbitz @kraftbj @leogermani